Add Scala Native support #2846

jchyb · 2021-11-05T00:26:53Z

Changes

Project structure was changed in compliance with sbt-crossproject.
googlecode's diffutils had to be removed, as Scala Native does not support java libraries. In its place, part of diffutils was reimplemented in scala as part of scalafmt-cli.
For similar reasons, the java interfaces were reimplemented in scala for Scala Native.
Since Scala Native does not yet support glob expressions, a translation function to regex was added, heavily based on a linked stack-overflow answer. Similarly, Scala's Process is replaced with java stdlib's ProcessBuilder, as Process uses concurrency.
SN's regex implementation is currently based on the re2 instead of the standard java regexes. This means there are a number of differences between them, most notably the lack of backtracking in Scala Native. To accommodate that, some regex function reimplementations were done to match the use of the existing scalafmt regexes in Scala Native.
Some operations concerning tests were slightly modified to work on Native. For example, renaming a file to an existing directory path could throw errors. Hopefully this is fine, as it does not concern the core of the test, only things like cleanup.
Assumptions about dynamic tests not working on native were added. Scala Native does not support URLs and only has basic opt-in reflection support comparable to the one of GraalVM, so the used and expected version of scalafmt must match here as well.
Term Display was made single-threaded for the Scala Native, as SN does not support concurrency yet. Since the new implementation caused display artifacts on JVM, the old multithreaded one was kept there.
Scala Native setup and testing was added to the CI. Windows support was temporarily disabled as it does not pass all of the tests yet.
scala-native sbt command was also added for easier scala-native cli building. Use sbt scala-native to generate Scala Native binaries (with LLVM installed).

Publishing

I did not include any publishing in this PR. For now there is not much point in releasing the scala-native binaries. Publishing as a library is another story, but I have not looked into that yet.

Performance

There are some severe performance problems when using the binaries generated from Scala Native. This should not happen. I tried to move the regex reimplementation as well as TermDisplay to use in JVM, yet that did not cause any noticeable performance problems there - unfortunately it seems that there is a bug on scala-native side. I will try to look into that issue.

Comparison running on Scalafmt repo:

JVM:

VL-D-0143:scalafmt jchyb$ time java -jar ./scalafmt-cli/jvm/target/scala-2.13/scalafmt.jar
Reformatting...
  100,0% [##########] 217 source files formatted

real    0m8.961s
user    0m43.648s
sys     0m1.467s

Native (Immix GC, release-fast optimization mode, full LTO, but I tried with many more options to no avail):

VL-D-0143:scalafmt jchyb$ time ./scalafmt-cli/native/target/scala-2.13/scalafmt-cli-out
Reformatting...
  100.0% [##########] 217 source files formatted

real    0m11.005s
user    0m15.570s
sys     0m5.000s

Additional comments

To smoothen out the review process I split this PR into two commits, one with only renames, and one with more meaningful changes. Hopefully this will help a bit :)

Project structure was changed in compliance with sbt-crossproject to accomodate Scala Native support. Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>

Project structure was changed in compliance with sbt-crossproject. googlecode's diffutils had to be removed, as Scala Native does not support java libraries. In its place, part of diffutils was reimplemented in scala as part of scalafmt-cli. For similar reasons, the java interfaces were reimplemented in scala for Scala Native. Since Scala Native does not yet support glob expressions, a translation function to regex was added, heavily based on a linked stack-overflow answer. Similarly, Scala's Process is replaced with java stdlib's ProcessBuilder, as Process uses concurrency. SN's regex implementation is currently based on the re2 instead of the standard java regexes. This means there are a number of differences between them, most notably the lack of backtracking in Scala Native. To accomodate that, some regex function reimplementations were done to accomodate the use of the existing scalafmt regexes in Scala Native. Some operations concerning tests were slightly modified to work on Native. For example, renaming a file to an existing directory path could throw errors. This is fine, as it does not concern the core of the test, only things like cleanup. Assumptions about dynamic tests not working on native were added. Scala Native does not support URLs and only has basic reflection support comparable to the one of GraalVM, so the used and expected version of scalafmt must match here as well. Term Display was made single-threaded for the Scala Native implementation, as it does not support concurrency. Since the new implementation caused display artifacts on JVM, the old multithreaded one was kept there. Scala Native setup and testing was added to the CI. The windows was temporarily disabled as it does not pass all of the tests. scala-native sbt command was also added for easier scala-native cli building. Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>

kitbellew · 2021-11-05T00:46:46Z

@jchyb massive amount of work, kudos!

i like your decision to rename in a separate commit. I'd like to suggest that we split the second one into several more focused commits, such as turning projects into cross projects, adding each replacement one at a time (for diffutils etc.).

in fact, maybe even split into a couple of pull requests. reviewing 300 files is scary. 😀

jchyb · 2021-11-05T10:04:43Z

Most of the work was honestly done by Tomasz, as was it his idea to do a separate commit. I can of course split it into further commits with files concerning separate changes later today. Personally, I would prefer keeping it all in this PR, as I fear the merging could otherwise become messy and now its easier to rebase if necessary. In case of potential review findings/concerns, I can be updating the individual commits, so that we don't have to use the now-dreaded "Files changed" tab ;)

kitbellew

very, very incomplete review. definitely lots of changes which can be submitted and merged separately.

also, would strongly prefer that every commit result in valid, compilable, testable code and could be separated or reverted by itself.

kitbellew · 2021-11-05T10:23:41Z

scalafmt-cli/jvm/src/main/scala/org/scalafmt/cli/UpdateDIsplayThread.scala

@@ -0,0 +1,262 @@
+package org.scalafmt.cli


file name itself contains a typo

is this copied from somewhere? complex code.

kitbellew · 2021-11-05T10:24:48Z

docs/contributing-scalafmt.md

@@ -81,7 +81,7 @@ To build a native image of the command-line interface using

 - From the project root directory,
  - run `sbt cli/assembly`
-  - run `java -jar scalafmt-cli/target/scala-2.13/scalafmt.jar`, to execute recently built artifacts
+  - run `java -jar scalafmt-cli/jvm/target/scala-2.13/scalafmt.jar`, to execute recently built artifacts


i assumed that no suffix for jvm meant perhaps that jvm wouldn't be added here...

Unfortunately that's how cross building works in this case. We could probably override it for jvm?

No suffix only works for for the sbt commands I think, so f.e. in the cross project instead of cliJVM/run we only need to run cli/run for the JVM platform in this branch. The idea was to keep the previously used commands intact.

kitbellew · 2021-11-05T10:25:49Z

build.sbt

@@ -174,15 +210,6 @@ lazy val cli = project
        val oldStrategy = (assembly / assemblyMergeStrategy).value
        oldStrategy(x)
    },
-    libraryDependencies ++= Seq(
-      "com.googlecode.java-diff-utils" % "diffutils" % "1.3.0",
-      "com.martiansoftware" % "nailgun-server" % "0.9.1",


looks like this dependency remains. why not keep it here, instead pushing down?

That's probably my bad, experimented a lot with the project and kind of left a mess.

i must have misunderstood what that no suffix call does. just wanted to make sure this is correct.

kitbellew · 2021-11-05T10:30:05Z

scalafmt-cli/native/src/main/scala/org/scalafmt/cli/UpdateDisplayThread.scala

@@ -0,0 +1,280 @@
+package org.scalafmt.cli


nothing in common with the other version?

kitbellew · 2021-11-05T10:32:31Z

scalafmt-cli/shared/src/main/scala/org/scalafmt/cli/ScalafmtCoreRunner.scala

@@ -22,11 +22,10 @@ object ScalafmtCoreRunner extends ScalafmtRunner {
      ExitCode.UnexpectedError
    } { scalafmtConf =>
      options.common.debug.println(s"parsed config (v${Versions.version})")
-      val filterMatcher = ProjectFiles.FileMatcher(
+      lazy val filterMatcher = ProjectFiles.FileMatcher(


why is this needed?

kitbellew · 2021-11-05T11:10:52Z

scalafmt-interfaces/native/src/main/scala/org/scalafmt/interfaces/ScalafmtSession.scala

@@ -0,0 +1,26 @@
+package org.scalafmt.interfaces


what's the difference with non native version? should we simply move from Java to scala in the original?

kitbellew · 2021-11-05T11:11:37Z

scalafmt-tests/shared/src/test/scala/org/scalafmt/cli/CliOptionsTest.scala

+    // native - FileNotFoundException, JVM - NoSuchFileException
+    assert(
+      confError.cause.exists(err =>
+        err.isInstanceOf[NoSuchFileException] || err


break line, looks bad

kitbellew · 2021-11-05T11:12:40Z

scalafmt-tests/shared/src/test/scala/org/scalafmt/cli/CliOptionsTest.scala

@@ -104,7 +105,13 @@ class CliOptionsTest extends FunSuite {
    val opt = baseCliOptions.copy(config = Some(configPath.toFile))
    assert(opt.scalafmtConfig.isInstanceOf[Configured.NotOk])
    val confError = opt.scalafmtConfig.asInstanceOf[Configured.NotOk].error
-    assert(confError.cause.exists(_.isInstanceOf[NoSuchFileException]))
+    // native - FileNotFoundException, JVM - NoSuchFileException


and we can't use the same in both cases?

@jchyb If you haven't already, an issue should be filed in Scala Native.

kitbellew · 2021-11-05T11:15:19Z

scalafmt-tests/shared/src/test/scala/org/scalafmt/cli/CliTest.scala

-    val label = if (version == Versions.version) "core" else "dynamic"
+    val (label, isDynamic) =
+      if (version == Versions.version) ("core", false) else ("dynamic", true)
+    def assumeNotDynamicOnNative(): Unit =


can't we just call this once?

kitbellew · 2021-11-05T11:17:13Z

scalafmt-tests/shared/src/test/scala/org/scalafmt/util/GitOpsTest.scala

@@ -70,9 +70,10 @@ class GitOpsTest extends FunSuite {
      dir.orElse(ops.rootDir).get.jfile.toPath,
      "dir_"
    )
+    val destPath = destDir.resolve("new_file")


looks like perfect candidate for separate pr

tgodzik · 2021-11-05T16:03:59Z

Thanks @jchyb for following up with the PR and thanks @kitbellew for the review! I don't expect to have this merged right away and we should come up with a plan what are the parts that we can merge separately and ones that we can split over multiple commits.

I will talk with @jchyb how we can go about it.

tgodzik · 2021-11-05T16:08:14Z

Just thinking about it now we could split it at least into:

adding difflib (maybe make it a separate library)
adding cross project without native support (so msotly renames)
adding other native changes in separate commits

kitbellew · 2021-11-05T16:12:32Z

Just thinking about it now we could split it at least into:

adding difflib (maybe make it a separate library)

great idea (both)

adding cross project without native support (so msotly renames)

preparing jvm changes for native counterpart as part of that (such as regex refactors, term display, sys process etc.)

adding other native changes in separate commits

He-Pin · 2023-08-30T14:14:59Z

is this still possible to get merged?

tgodzik · 2023-08-30T15:50:17Z

I think this requires a bit more time, which we currently don't have, so if anyone would be willing to spend some time on it that would be awesome.

ekrich · 2023-08-30T16:15:36Z

Getting the difflib made into a scalameta project would be great. I asked Wojciech if it might work for our uses in Scala Native too.

He-Pin · 2023-08-30T16:27:38Z

I think this PR is big ,maybe can be splitter and deliver in batchs.

ekrich · 2023-08-30T16:40:06Z

@He-Pin Yes, that is what @kitbellew suggests above. The difflib might be able to be used by Scala Native too. Not sure the best way there - create a project for eventual adding to scalameta org? @tgodzik What do you think?

tgodzik · 2023-08-31T09:45:39Z

@He-Pin Yes, that is what @kitbellew suggests above. The difflib might be able to be used by Scala Native too. Not sure the best way there - create a project for eventual adding to scalameta org? @tgodzik What do you think?

We usually inline it as it's rather small, so we never had any will to make it a separate library.

jchyb and others added 2 commits November 4, 2021 19:42

Change project structure for SN support - renames only

dedb624

Project structure was changed in compliance with sbt-crossproject to accomodate Scala Native support. Co-authored-by: Tomasz Godzik <tgodzik@users.noreply.github.com>

jchyb force-pushed the feature/add-scala-native branch from 69f6f29 to 5b48eed Compare November 5, 2021 00:27

kitbellew requested a review from tgodzik November 5, 2021 00:42

kitbellew requested changes Nov 5, 2021

View reviewed changes

armanbilge mentioned this pull request Aug 26, 2022

WIP: Fix #2798: Read System Properties from an environment variable scala-native/scala-native#2800

Closed

kitbellew mentioned this pull request Mar 5, 2023

Adoption of AUR package but ran into issues #3490

Open

Gedochao mentioned this pull request Apr 12, 2024

scala-cli fmt not working with rewrite. VirtusLab/scala-cli#2684

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Scala Native support #2846

Add Scala Native support #2846

jchyb commented Nov 5, 2021

kitbellew commented Nov 5, 2021

jchyb commented Nov 5, 2021

kitbellew left a comment

kitbellew Nov 5, 2021

kitbellew Nov 5, 2021

tgodzik Nov 5, 2021

jchyb Nov 8, 2021

kitbellew Nov 5, 2021

tgodzik Nov 5, 2021

kitbellew Nov 5, 2021

kitbellew Nov 5, 2021

kitbellew Nov 5, 2021

kitbellew Nov 5, 2021

kitbellew Nov 5, 2021

kitbellew Nov 5, 2021

ekrich Jan 15, 2022

kitbellew Nov 5, 2021

kitbellew Nov 5, 2021

tgodzik commented Nov 5, 2021

tgodzik commented Nov 5, 2021

kitbellew commented Nov 5, 2021

He-Pin commented Aug 30, 2023

tgodzik commented Aug 30, 2023

ekrich commented Aug 30, 2023

He-Pin commented Aug 30, 2023

ekrich commented Aug 30, 2023

tgodzik commented Aug 31, 2023

Add Scala Native support #2846

Are you sure you want to change the base?

Add Scala Native support #2846

Conversation

jchyb commented Nov 5, 2021

Changes

Publishing

Performance

Additional comments

kitbellew commented Nov 5, 2021

jchyb commented Nov 5, 2021

kitbellew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Nov 5, 2021

tgodzik commented Nov 5, 2021

kitbellew commented Nov 5, 2021

He-Pin commented Aug 30, 2023

tgodzik commented Aug 30, 2023

ekrich commented Aug 30, 2023

He-Pin commented Aug 30, 2023

ekrich commented Aug 30, 2023

tgodzik commented Aug 31, 2023